WIP: Add getChunk method to FrameSet and substitute its result into commands #367
Conversation
|
Hi @donalm, sorry for the radio silence here. It's a bit of a complex PR and we just haven't yet been able to block out time for it yet! But it's on our radar. |
|
No worries - thanks @bcipriano |
| * @param zFill The required width of each left-padded frame ID, e.g. 4 if ID '1' should become '0001' | ||
| * @return A representation of a frameset, e.g. '1-10,12-100x2' | ||
| */ | ||
| private String framesToFrameRanges(ImmutableList<Integer> frames, int zFill) { |
There was a problem hiding this comment.
It doesn't look like there's a way to use zFill at the moment, is there? Since these are all private methods, and the public methods don't have a corresponding argument. Should we just drop it?
If you think it's worth keeping it around, let's add tests that utilize it.
There was a problem hiding this comment.
I removed all the zFill and pad method stuff that's not really being used.
| sb.append(resultBuilder.get(j)).append(","); | ||
| } | ||
| sb.append(resultBuilder.get(l)); | ||
| return sb.toString(); |
There was a problem hiding this comment.
Here's a slight rewrite of this block which uses Java 8 streams and a StringJoiner to handle adding the delimiters.
StringJoiner sj = new StringJoiner(",");
resultBuilder.forEach(sj::add);
return sj.toString();
There was a problem hiding this comment.
Ah thanks I was was looking for something like this.
cuebot/src/test/java/com/imageworks/spcue/test/util/FrameSetTests.java
Outdated
Show resolved
Hide resolved
| return pad(frames.get(0), zFill); | ||
| } | ||
|
|
||
| ArrayList<String> resultBuilder = new ArrayList<String>(); |
There was a problem hiding this comment.
You can drop the second String, it's redundant.
ArrayList<String> resultBuilder = new ArrayList<>();
There was a problem hiding this comment.
Turns out I don't need this variable now because I'm just using StringJoiner directly
|
Thanks for the review Brian. I think I addressed everything you raised but please bounce it back to me if there's still some stuff to improve. |
There was a problem hiding this comment.
LGTM!
@donalm Were you able to resolve any issues with the CLA? I noticed on #434 that it was throwing a warning.
@gregdenton Would you mind taking a quick look at this PR as well?
|
Aye the CLA got resolved and the repo seemed to recognise that without any intervention which was nice. I think this just needs the nod from @gregdenton now. Thanks @bcipriano |
|
That's great! Thanks everyone. |
**Link the Issue(s) this Pull Request is related to.** #1129 **Summarize your change.** Fix the frame end resolution of a frame chunk used to fill the place holder `#FRAME_END#`. The current behavior incorrectly returns the index of the last frame in the frame list instead of its value. Leverage `FrameSet.get_chunk` method that is already doing all the legwork to get the last frame. **Related topics** - #1320 - #367 --------- Signed-off-by: Anton Brand <[email protected]> Co-authored-by: Kern Attila GERMAIN <[email protected]>
Submitted as a WIP because:
But - it works well enough for me that I can proceed with testing.
This update allows me to submit a command like this:
/bin/true -f #FRAMESPEC#And the rqd daemon is assigned something like this (depending on framerange/chunk size):
The logic is ported directly from FileSeq.